Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

refactor(ngAnimateSwap): remove unused dependency and add tests #16565

Closed

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented May 15, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Refactor.
Tests.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

@@ -132,6 +132,51 @@ describe('ngAnimateSwap', function() {
expect(two).toBeTruthy();
}));

it('should create a new (non-isolate) scope for each inserted clone', inject(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this test proves that the new scopes are non-isolate?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't an isolate scope be directly derived from the rootScope and make this fail: expect(scopeOne.$parent).toBe(parentScope)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - an isolate scope still has a parent but it doesn't inherit from its parent. So

$parent.foo = 'bar';
$isoChild = $parent.$new(true);
assert($isoChild.foo !== 'bar');
assert($isoChild.$parent.foo === 'bar');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops 😁

expect(scopeTwo.$$destroyed).toBe(true);
}));

it('should destroy the previous scope before when swapping elements', inject(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... before ...?

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM a couple of minor points

@Narretz
Copy link
Contributor

Narretz commented May 16, 2018

LGTM, but the test commit should be another refactor because you've changed the implementation a bit.

@gkalpak gkalpak force-pushed the refactor-ngAnimateSwap-unused-dep branch from 21f5b6c to 431dc5e Compare May 16, 2018 16:10
@gkalpak gkalpak closed this in f6e189d May 16, 2018
@gkalpak gkalpak deleted the refactor-ngAnimateSwap-unused-dep branch May 23, 2018 19:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants